Skip to content

fix: guard nil DefaultLogger.Out in moduleReload teardown (#846)#849

Closed
SAY-5 wants to merge 10 commits into
foxcpp:devfrom
SAY-5:fix-reload-nil-logger
Closed

fix: guard nil DefaultLogger.Out in moduleReload teardown (#846)#849
SAY-5 wants to merge 10 commits into
foxcpp:devfrom
SAY-5:fix-reload-nil-logger

Conversation

@SAY-5
Copy link
Copy Markdown

@SAY-5 SAY-5 commented May 21, 2026

Fixes #846.

systemctl reload maddy (SIGUSR2) crashes the daemon with a nil pointer dereference in the async teardown goroutine of moduleReload. The new server has already bound listeners, but the panicking goroutine takes the whole process down with it, leaving the host with no mail service running until systemctl restart maddy is issued.

Root cause: when the configuration has no top-level log directive, defaultLogOutput returns (nil, nil) and c.DefaultLogger.Out is left unset. The moduleMain shutdown path at maddy.go:452 already nil-checks before calling Close(); the reload teardown goroutine added in 5841e95 (log: Refactor to define proper loggers tree) did not, so the close at maddy.go:520 panics.

Fix: factor the close into a small closeContainerLogOutput helper that mirrors the existing nil-safe shutdown pattern, and call it from the reload teardown goroutine.

Reproducer (boots a container with no log directive and runs the same Out.Close() call that the teardown goroutine made):

c := &container.C{DefaultLogger: &log.Logger{}} // Out is nil
_ = c.DefaultLogger.Out.Close()
// panic: runtime error: invalid memory address or nil pointer dereference

Tests:

  • TestCloseContainerLogOutput_NilOut — exercises the bug condition; before this change the equivalent inline Out.Close() panicked.
  • TestCloseContainerLogOutput_NonNil — happy path is unchanged.
  • TestCloseContainerLogOutput_CloseError — Close errors are still surfaced through the new container's logger.
  • TestCloseContainerLogOutput_NilContainer — defensive coverage for nil container / nil logger.

go test -race . and gofmt -l . are clean.

foxcpp and others added 10 commits May 16, 2026 01:24
* add Logger.Close(), which safely closes underlying Out
* should fix foxcpp#846
SystemD would report maddy in "reloading (reload-notify)" state even after
successful reload. It should send "READY=1" after finishing the reload.
When the configuration has no top-level 'log' directive, defaultLogOutput
returns (nil, nil) and the container's DefaultLogger.Out is left unset.
The async teardown goroutine in moduleReload called Out.Close()
unconditionally, so 'systemctl reload maddy' (SIGUSR2) panicked and the
daemon exited with status=2/INVALIDARGUMENT, dropping the mail service.

Mirror the nil-check used by the synchronous shutdown path in moduleMain
by extracting the close into a small helper and calling it from the
reload teardown goroutine. Add unit tests covering the nil-Out, non-nil,
Close-error, and nil-container code paths.

Fixes foxcpp#846.
fix(systemd): report READY=1 after reload
fix: check for nil Out in Logger on Close()
@SAY-5
Copy link
Copy Markdown
Author

SAY-5 commented May 28, 2026

Closing, #847 fixed this at the Logger.Close() layer so the moduleReload teardown is already safe. Thanks.

@SAY-5 SAY-5 closed this May 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants